fix(action): scope exit-code capture instead of disabling errexit globally#13
Merged
Conversation
…bally Addresses the Sourcery review on #12 (the set +e change is already in main via #11, released in v1.6.0): - action.yml: replace `set +e` + direct call with `EXIT_CODE=0; leakwatch "${ARGS[@]}" || EXIT_CODE=$?`, so errexit stays enabled for the rest of the step (later failures still fail fast) while a findings exit (1) is captured and mapped. Guard the job-summary jq pipe with `|| true` so a malformed SARIF can't abort before the exit-code mapping. - action-test.yml (cli-github-format): capture the exit code instead of `|| true`, failing on a real error (>=2) while tolerating findings (0/1). Verified under `bash -e -o pipefail`: the mapping runs and a subsequent failing command still aborts (errexit not globally disabled). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideScopes exit-code handling for the leakwatch scan and GitHub-format test so that bash errexit remains enabled globally while still correctly distinguishing expected findings (exit 1) from real errors, and ensures job-summary rendering failures do not mask scan results. Flow diagram for cli-github-format test exit-code handlingflowchart TD
A[Start cli-github-format job] --> B[Run leakwatch GitHub format scan and capture exit code]
B --> C{EXIT_CODE value}
C --> C0[EXIT_CODE=0: success]
C --> C1[EXIT_CODE=1: findings tolerated]
C --> C2[EXIT_CODE>=2: real error]
C0 --> D[Job continues and passes]
C1 --> D
C2 --> E[Job fails to surface hard error]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCapture leakwatch exit codes without disabling errexit, add a non-fatal fallback for SARIF findings rendering, and update the workflow test to treat ChangesExit Code Handling and Error Resilience
Sequence Diagram(s)sequenceDiagram
participant ActionRunner
participant Leakwatch
participant JobSummary
ActionRunner->>Leakwatch: run leakwatch with ARGS
Leakwatch-->>ActionRunner: exit code (0/1/>=2)
ActionRunner->>ActionRunner: set EXIT_CODE or rc based on result
ActionRunner->>JobSummary: jq render -> temp file -> head -50 or fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the workflow test step, the pattern
out="$(${RUNNER_TEMP}/leakwatch ...)" || rc=$?relies on the subtle behavior that the assignment’s exit status mirrors the command substitution; consider splitting this into an explicitifblock (e.g.if ! out="$(${...})"; then rc=$?; fi) to make the error-handling semantics clearer and less surprising to future readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the workflow test step, the pattern `out="$(${RUNNER_TEMP}/leakwatch ...)" || rc=$?` relies on the subtle behavior that the assignment’s exit status mirrors the command substitution; consider splitting this into an explicit `if` block (e.g. `if ! out="$(${...})"; then rc=$?; fi`) to make the error-handling semantics clearer and less surprising to future readers.
## Individual Comments
### Comment 1
<location path="action.yml" line_range="313" />
<code_context>
echo "| Level | Detector | Location |"
echo "| --- | --- | --- |"
- jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50
+ jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || true
if [ "${total:-0}" -gt 50 ] 2>/dev/null; then
echo ""
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `|| true` here could over-mask genuine issues in the summary generation
This makes the summary step non-fatal for malformed/missing `$OUT`, but it also hides real failures (e.g., `jq` not installed, unexpected output shape) that we’d want to notice. To keep resilience without fully swallowing errors, consider logging a clear failure message instead (e.g., `... || echo "Failed to render findings summary from $OUT" >&2`).
```suggestion
jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || echo "Failed to render findings summary from \"${OUT:-<unset>}\" (jq exit code: $?)" >&2
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors error handling in the action script by capturing the exit code of the leakwatch scan locally instead of disabling global errexit, ensuring subsequent commands still fail fast. It also adds a safety || true to the jq pipeline for the job summary. Feedback suggests further hardening the jq filter using the optional operator to handle potentially missing metadata in SARIF results.
| echo "| Level | Detector | Location |" | ||
| echo "| --- | --- | --- |" | ||
| jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 | ||
| jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || true |
There was a problem hiding this comment.
While adding || true prevents the script from aborting on jq or SIGPIPE errors, the jq filter itself is still fragile. If a SARIF result is missing the locations array or any nested property (e.g., for a global finding), jq will throw an error like Cannot index null with number and stop processing further results.
You can use the ? operator to make the filter more robust, ensuring the job summary table is populated even if some findings have incomplete metadata.
jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri)? // "-")\(if (.locations[0].physicalLocation.region.startLine)? then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || true
Collaborator
Author
There was a problem hiding this comment.
Verified this isn't needed for this filter: jq doesn't error on a result without locations because every accessed field is defaulted with // "-". Indexing null returns null in jq (not an error), and null // "-" → "-":
$ echo '{"runs":[{"results":[{"level":"warning","ruleId":"slack-token"}]}]}' | \
jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))… |"'
| warning | slack-token | - | # exit 0
So the ? operator would be redundant. Keeping the filter as-is (the // "-" defaults already make it null-safe).
Follow-up to the PR #13 review: - action.yml: replace the job-summary `|| true` with a visible fallback note so a (theoretical) render failure isn't silently swallowed. The jq filter already handles location-less findings via `// "-"` (verified jq returns "-" without erroring), so the `?` operator is unnecessary. - action-test.yml (cli-github-format): capture the exit code with an explicit if/else to make the semantics obvious. (The reviewer's `if ! out=…; then rc=$?` form is incorrect — it captures 0, not the real exit code.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thanks @sourcery-ai. On the test-step exit-code capture: switched to an explicit if out="$("${RUNNER_TEMP}/leakwatch" … )"; then rc=0; else rc=$?; fiHeads-up on the suggested The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Line 313: The current pipeline "jq ... | head -50 || echo ..." can trigger the
fallback due to a broken pipe even when jq produced output; instead run jq to a
temporary file first (e.g., produce the table into a temp artifact), then
display the first 50 lines from that file with "head -n 50" and only emit the
fallback if the temp file is empty or head produced no output—update the command
that uses jq and head (the "jq -r '... | head -50 || echo ...'") to a two-step
approach: write jq output to a temp file, then run head -n 50 on that temp file
and conditionally echo the fallback if the file is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d16dad14-38a0-45c0-b244-0d02548cdf31
📒 Files selected for processing (2)
.github/workflows/action-test.ymlaction.yml
…e fallback Piping jq into `head -50` gives jq a SIGPIPE once head closes after 50 lines; under pipefail that non-zero status tripped the `|| echo fallback` even when the table rendered fine (reproducible once jq's output exceeds the ~64KB pipe buffer, i.e. very many findings). Write jq output to a temp file, then `head -n 50` it, and emit the fallback only when jq fails or the file is empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the Sourcery review on #12.
#12's underlying change (the
set +efix for findings exiting 1 underbash -e) is already inmain— it was merged via #11 and shipped in v1.6.0 — so #12 itself is redundant (and would conflict). This PR applies Sourcery's two valid follow-up suggestions tomain:action.ymlnow usesEXIT_CODE=0; leakwatch … || EXIT_CODE=$?instead ofset +e, so-estays on for the rest of the step (later command failures still fail fast) while findings (exit 1) are captured and mapped. The job-summaryjq | headpipe is guarded with|| trueso a malformed SARIF can't abort the step before the exit-code mapping.cli-github-formatjob captures the exit code and fails on a hard error (>=2) while tolerating findings (0/1), instead of|| true.Verification
Reproduced under
bash -e -o pipefail: the exit-code mapping runs and a subsequent failing command still aborts the step (errexit is no longer globally disabled).shellcheck(both run scripts) +actionlintclean.🤖 Generated with Claude Code
Summary by Sourcery
Scope leakwatch exit-code capture to preserve errexit behavior while still honoring fail-on-findings configuration, and tighten tests to distinguish expected findings from hard errors.
Bug Fixes:
Tests:
Summary by CodeRabbit